Skip to content

bump controller-tools to v0.2.0#1642

Merged
estroz merged 17 commits intooperator-framework:refactor/controller-runtime-v0.2.0from
estroz:bump-controller-tools-v0.2.0
Aug 29, 2019
Merged

bump controller-tools to v0.2.0#1642
estroz merged 17 commits intooperator-framework:refactor/controller-runtime-v0.2.0from
estroz:bump-controller-tools-v0.2.0

Conversation

@estroz
Copy link
Copy Markdown
Member

@estroz estroz commented Jul 3, 2019 •
edited
Loading

Description of the change: bump controller-tools to v0.2.0 and update CRD generator API/unit tests.

Motivation for the change: controller-tools k8s deps versions should match those of controller-runtime when possible. #1143 requires some changes in this release too.

Note: #1607 should be closed once the refactor base branch is merged with master.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 3, 2019
@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 3, 2019
@estroz estroz force-pushed the bump-controller-tools-v0.2.0 branch 2 times, most recently from ba3db71 to c49ecbb Compare July 4, 2019 15:21
@estroz estroz changed the title [WIP] bump controller-tools to v0.2.0 bump controller-tools to v0.2.0 Jul 7, 2019
@estroz estroz requested review from AlexNPavel, hasbro17 and lilic July 7, 2019 01:16
lilic
lilic suggested changes Jul 8, 2019
View reviewed changes
Comment thread internal/pkg/scaffold/crd.go
Comment thread internal/pkg/scaffold/crd_test.go
@estroz estroz requested a review from lilic July 8, 2019 18:30
@estroz estroz force-pushed the bump-controller-tools-v0.2.0 branch from c86b140 to 8f7bfbd Compare July 8, 2019 19:12
@estroz
Copy link
Copy Markdown
Member Author

estroz commented Jul 8, 2019 •
edited
Loading

Two changes that need discussion:

  1. CRD manifests can contain multiple specified versions, so CRD file naming should change from <group>_<version>_<kind>_crd.yaml to something else. For example, controller-tools uses <group>_<resource>.yaml for CRD's. To stick with our prior suffix convention and make it clear what the file contains I suggest <group>_<resource>_crd.yaml.
  2. The CRD generator permits legacy support for spec.version but not simultaneously with spec.versions. The checkCRDVersions() function permits both forms as long as spec.version is the first element in spec.versions, which is correct behavior. How should we handle this discrepancy? Do we need to do anything aside from log a warning about spec.version deprecation and error if the expected versioning spec is not met?

/cc @joelanford @AlexNPavel @hasbro17 @lilic

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 8, 2019
@hasbro17
Copy link
Copy Markdown
Contributor

hasbro17 commented Jul 9, 2019

@estroz Can we rebase or cherry pick #1652 into the refactor/controller-runtime-v0.2.0 branch?
And update the tag from // +genclient:nonNamespaced to // +kubebuilder:resource:path=<lowercase-kind-plural>,scope=Namespaced

Or do you want to do that as a separate PR?

@lilic
Copy link
Copy Markdown
Member

lilic commented Jul 9, 2019

@estroz For .1 I think we should follow the convention that kubebuilder has, so it will make it easier to migrate, do they have anything established already?

For 2. I think the log about the deprecation and error out if spec is not met, sounds good to me.

lilic
lilic approved these changes Jul 9, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@lilic lilic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we update the CHANGELOG.

@hasbro17 if you are the one doing the migration guide we should make a note of the versions/version in there as well.

Otherwise lgtm

@estroz estroz force-pushed the bump-controller-tools-v0.2.0 branch from 9576718 to 6d4d6c3 Compare July 9, 2019 18:34
@estroz
Copy link
Copy Markdown
Member Author

estroz commented Jul 9, 2019

CRD file naming changes were quite extensive and touched both Helm and Ansible docs/tests. PTAL.

/cc @lilic @hasbro17 @AlexNPavel @joelanford @jmrodri @fabianvf

@estroz estroz force-pushed the bump-controller-tools-v0.2.0 branch 3 times, most recently from 237857f to 90d0b3f Compare July 9, 2019 22:31
@estroz
Copy link
Copy Markdown
Member Author

estroz commented Jul 10, 2019

@lilic kubebuilder would generate a CRD file name cache.example.com_memcacheds.yaml. IMO cache_memcacheds_crd.yaml looks much nicer and contains all the necessary uniqueness/information that the kubebuilder name gives. WDYT?

hasbro17
hasbro17 reviewed Jul 10, 2019
View reviewed changes
Comment thread CHANGELOG.md Outdated
@lilic
Copy link
Copy Markdown
Member

lilic commented Jul 10, 2019

@lilic kubebuilder would generate a CRD file name cache.example.com_memcacheds.yaml. IMO cache_memcacheds_crd.yaml looks much nicer and contains all the necessary uniqueness/information that the kubebuilder name gives. WDYT?

Since we are moving to kubebuilder at some point, I would suggest we go the kubebuilder way as we probably will have to change it then anyways.

@estroz estroz force-pushed the bump-controller-tools-v0.2.0 branch from 26bcf4b to 3b49157 Compare July 10, 2019 23:09
@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 5, 2019
@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 5, 2019
This was referenced Aug 5, 2019
Merged
Merged
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 6, 2019
@estroz estroz mentioned this pull request Aug 10, 2019
Merged
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 12, 2019
@estroz estroz requested review from hasbro17 and joelanford August 12, 2019 18:03
joelanford
joelanford reviewed Aug 23, 2019
View reviewed changes
Comment thread internal/pkg/scaffold/crd.go Outdated
Comment thread internal/pkg/scaffold/crd.go
Comment thread internal/pkg/scaffold/crd.go
Comment thread internal/pkg/scaffold/go_mod.go Outdated
@estroz estroz requested a review from joelanford August 26, 2019 18:28
joelanford
joelanford approved these changes Aug 27, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 27, 2019
hasbro17
hasbro17 approved these changes Aug 27, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@hasbro17 hasbro17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@estroz estroz merged commit 4d330f3 into operator-framework:refactor/controller-runtime-v0.2.0 Aug 29, 2019
@estroz estroz deleted the bump-controller-tools-v0.2.0 branch August 29, 2019 16:17
@estroz estroz mentioned this pull request Sep 4, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joelanford joelanford joelanford approved these changes

@fabianvf fabianvf Awaiting requested review from fabianvf

@AlexNPavel AlexNPavel Awaiting requested review from AlexNPavel

@jmrodri jmrodri Awaiting requested review from jmrodri

@lilic lilic Awaiting requested review from lilic

@theishshah theishshah Awaiting requested review from theishshah

+1 more reviewer

@hasbro17 hasbro17 hasbro17 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@joelanford joelanford

Labels

lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@estroz @hasbro17 @lilic @joelanford @openshift-ci-robot